-
Notifications
You must be signed in to change notification settings - Fork 545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update warning when users sign images by tag. #2313
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2313 +/- ##
==========================================
- Coverage 30.06% 30.05% -0.02%
==========================================
Files 131 131
Lines 8062 8066 +4
==========================================
Hits 2424 2424
- Misses 5314 5318 +4
Partials 324 324
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
See #2314 for lint. Should be fixed once that's merged and I rebase |
aaf38ee
to
1890cc5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'm wondering if we should add a NOTE
in the README, so any newbies are aware of this security recommendation.
Newest change covers a lot of docs, and also adds some tests. |
will close/reopen to see if that retriggers the ci, otherwise we will need some rebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you
See sigstore#2047. Signed-off-by: Zachary Newman <[email protected]>
Signed-off-by: Zachary Newman <[email protected]>
Also explicitly check for Digest being set, rather than Tag not being set. This doesn't actually make a difference because name.ParseReference just throws away the tag in such cases (maybe a bug), but it does make the intent clearer. Signed-off-by: Zachary Newman <[email protected]>
had to trigger tests 1 more time because the DCO check hung....fingers crossed! |
no substantive changes since hector last approved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, thanks 💪
See #2047.
Summary
Signing by tag can lead to race conditions and possibly insecure behavior, signing images other than those intended. This PR adds a warning when people do that (later, we'll make it an error.)
Release Note
Added warning when users refer to images to sign by tag (
example.com/image:latest
). Instead, refer to images by digest (example.com/image@sha256:abcdef
). This will be required in a future release.Documentation
No change to web docs; changes to CLI docs included.